-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade @svgr/webpack to 4.1.0 #5816
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
This was on my todo list so thanks for taking care if it! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I fixed the failing tests on Travis, but not sure why appveyor is failing, looks like it's timing out. I added the babel packages to resolve the failing tests, but if there's a more appropriate way to add to the project, I'm all ears. |
Appveyor had been broken for a while so don't worry about that. I'm more concerned about the fact that we have to add all of these babel dependencies. It's also weird that they seem to only be required in ejected projects. @neoziro do you have any idea why this might be? |
@iansu I don’t know why they are required, probably babel sub dependencies. But I think it is lighter than before. |
(SVGO is included in @svgr/webpack and I think it should be part of cra). It could reduce the bundle size. |
In your PR, for now it is included. |
That looks like a mistake on my part, I misunderstood how svgo was being included. Currently it appears to be intentionally left out, do you have a reason for excluding it @iansu? I updated the PR to remove prettier and svgo to feature-match with the existing implementation. Also trying to see if I can get away without a couple extra dependencies, since CI seems to be acting differently than my machine. |
Looks like SVGO was disabled intentionally., so I will leave it disabled unless there is a good reason to change it. Removing prettier and SVGO seems to have required another dependency for the kitchensink-eject tests to pass. Still not sure why that is the case or how to resolve that, so any additional help would be appreciated. |
I think I should be good to go now. My issue seems to have been resolved by merging in master which brought in |
@neoziro I don't think that's necessary after all. Updating the version of |
I think it is, the way plugin are imported could create conflict. If the user install babel + create-react-app or if he ejects. I prefer to have it fixed. The fix will be published in 5 minutes. |
@alaycock I think making config static and using |
@alaycock published as v4.0.4 |
I think we can remove |
You're right @iansu, I made that change. It looks like svgo could be disabled with |
@alaycock I think I published a v4.1.0, the only change that impacts you is an upgrade of unified / rehype. It is marked as minor because of the new parcel plugin. |
Hi, any update on this? I am experiencing an issue with the old version of @svgr/webpack, does anybody have a temporary workaround to offer? Thanks. |
@kopax I think this is good to go unless there's any more feedback. |
Ok. Maybe they should merge this and release a new CRA version for testing. Do you know how I can test this easely? Maybe we can replace react-script by a fork dist version on npm? |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs. |
@iansu, is there anything I should be doing to get this ready to merge? It sounds like some other PRs are waiting on this one. |
These build failures seem unrelated. We're currently trying to fix them: #6165. Once that's done we should be good to merge this. |
#6165 is in and we're all green. Let's do this! |
Thanks! |
I ran into an issue when upgrading to create-react-app 2 because
@svgr/core@2.4.1
(a dependency of@svgr/webpack
) had prettier as a dependency, which led to two different version of prettier being installed, which ended up causing linting issues when the two different versions of prettier ran (throughyarn eslint
vsyarn prettier
). More details of my issue can be found here: prettier/eslint-plugin-prettier#110As of
@svgr/core@4.0.0
,prettier
andsvgo
are no longer dependencies, and using it will prevent them from being installed, which would hopefully prevent prettier version conflicts in the future.Neither
prettier
andsvgo
were being used in create-react-app anyway, so having them removed just removes a bit ofnode_modules
bloat without cutting functionality.To test, run create react app, and try out importing a SVG as a component with
import { ReactComponent as Logo } from "./logo.svg";
, and it should continue to work.